Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build and ship an actual binary named wt that just launches WT #6860

Merged
2 commits merged into from
Jul 10, 2020

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Jul 10, 2020

Due to a shell limitation, Ctrl+Shift+Enter will not launch Windows
Terminal as Administrator. This is caused by the app execution alias and
the actual targeted executable not having the same name.

In addition, PowerShell has an issue detecting app execution aliases as
GUI/TUI applications. When you run wt from PowerShell, the shell will
wait for WT to exit before returning to the prompt. Having a shim that
immediately re-executes WindowsTerminal and then returns handily knocks
this issue out (as the process that PS was waiting for exits
immediately.)

This could cause a regression for anybody who tries to capture the PID
of wt.exe. Our process tree is not an API, and we have offered no
consistency guarantee on it.

VALIDATION

Tested manual launch in a number of different scenarios:

  • start menu "wtd"
  • start menu tile
  • powertoys run
  • powertoys run ctrl+shift (admin)
  • powershell inbox, "core"
  • cmd
  • run dialog
  • run dialog ctrl+shift (admin)
  • run from a lnk with window mode=maximized

Fixes #4645 (PowerShell waits for wt)
Fixes #6625 (Can't launch as admin using C-S-enter)

Due to a shell limitation, Ctrl+Shift+Enter will not launch Windows
Terminal as Administrator. This is caused by the app execution alias and
the actual targeted executable not having the same name.

In addition, PowerShell has an issue detecting app execution aliases as
GUI/TUI applications. When you run wt from PowerShell, the shell will
wait for WT to exit before returning to the prompt. Having a shim that
immediately re-executes WindowsTerminal and then returns handily knocks
this issue out (as the process that PS was waiting for exits
immediately.)

This could cause a regression for anybody who tries to capture the PID
of wt.exe. Our process tree is not an API, and we have offered no
consistency guarantee on it.

Fixes #4645 (PowerShell waits for wt)
Fixes #6625 (Can't launch as admin using C-S-enter)
@ghost ghost added Area-Commandline wt.exe's commandline arguments Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Jul 10, 2020

// Go!
wil::unique_process_information pi;
return CreateProcessW(module.c_str(), cmdline.data(), nullptr, nullptr, FALSE, 0, nullptr, nullptr, &si, &pi);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, createprocess returns 1 on success; definitely don't do this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider !!CreateProcessW

zadjii-msft
zadjii-msft previously approved these changes Jul 10, 2020
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate the hellscape we live in. I'm waiting for an answer before giving the ✔️


// Go!
wil::unique_process_information pi;
return CreateProcessW(module.c_str(), cmdline.data(), nullptr, nullptr, FALSE, 0, nullptr, nullptr, &si, &pi);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, the way this is, won't the commandline windowsterminal.exe processes be windownterminal.exe wt.exe new-tab ...?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually, no! The first argument to CreateProcess is the actual executable to run, and the second is its entire commandline. That includes the first argument, usually the program name, that comes in as argv[0].
There are some tools (usually linux ones) that detect what identity they were launched as and do something different depending on what it was. Busybox is a great example. It’s one binary that contains sh, ls, rm, cat, ... and at runtime decides which one to be based on how you invoked it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, all that is possible because the first argument, usually the program name, can vary and is detectable by the application. Because of that, though, it must be specified when creating a process!

@zadjii-msft zadjii-msft dismissed their stale review July 10, 2020 12:14

Whoops meant to comment, not approve

Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep OK.

@DHowett DHowett changed the title Build and ship an actual binary named wt/wtd that just launches WT Build and ship an actual binary named wt that just launches WT Jul 10, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 10, 2020
@ghost
Copy link

ghost commented Jul 10, 2020

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 592c634 into master Jul 10, 2020
@ghost ghost deleted the dev/duhowett/wt_shim branch July 10, 2020 22:41
@ghost
Copy link

ghost commented Jul 22, 2020

🎉Windows Terminal Preview v1.2.2022.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Commandline wt.exe's commandline arguments AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
3 participants